-
Notifications
You must be signed in to change notification settings - Fork 323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
upgrade(tracer): Support libdatadog's library_config module #5126
Conversation
Overall package sizeSelf size: 8.82 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.5.0 | 29.83 MB | 29.83 MB | | @datadog/native-appsec | 8.4.0 | 19.25 MB | 19.26 MB | | @datadog/native-iast-taint-tracking | 3.3.0 | 13.77 MB | 13.78 MB | | @datadog/pprof | 5.5.1 | 9.79 MB | 10.17 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.8.0 | 2.6 MB | 2.74 MB | | @opentelemetry/core | 1.14.0 | 872.87 kB | 1.47 MB | | @datadog/native-metrics | 3.1.0 | 1.06 MB | 1.46 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | import-in-the-middle | 1.11.2 | 112.74 kB | 835.4 kB | | source-map | 0.7.4 | 226 kB | 226 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | lru-cache | 7.18.3 | 133.92 kB | 133.92 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.0 | 109.9 kB | 109.9 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 5.3.2 | 53.63 kB | 53.63 kB | | shell-quote | 1.8.1 | 44.96 kB | 44.96 kB | | istanbul-lib-coverage | 3.2.0 | 29.34 kB | 29.34 kB | | rfdc | 1.3.1 | 25.21 kB | 25.21 kB | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | dc-polyfill | 0.1.4 | 23.1 kB | 23.1 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | semifies | 1.0.0 | 15.84 kB | 15.84 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | ttl-set | 1.0.0 | 4.61 kB | 9.69 kB | | path-to-regexp | 0.1.12 | 6.6 kB | 6.6 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | module-details-from-path | 1.0.3 | 4.47 kB | 4.47 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
BenchmarksBenchmark execution time: 2025-02-28 15:08:25 Comparing candidate commit e30985d in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 916 metrics, 17 unstable metrics. |
852b817
to
83d4172
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5126 +/- ##
==========================================
+ Coverage 80.64% 80.67% +0.02%
==========================================
Files 489 490 +1
Lines 21756 21836 +80
==========================================
+ Hits 17546 17617 +71
- Misses 4210 4219 +9 ☔ View full report in Codecov by Sentry. |
369cde4
to
d85addc
Compare
d4e0a29
to
f111eac
Compare
Datadog ReportBranch report: ✅ 0 Failed, 664 Passed, 0 Skipped, 10m 31.98s Total Time |
44e6e6f
to
58d3ea2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good, made some suggestions.
When @datadog/libdatadog
gets released, add package.json
and yarn.lock
updates to this PR – I know you mentioned in the PR description that it needs to be done; I'm just emphasizing that it should be done as part of this PR.
923400e
to
3361fe4
Compare
34870a4
to
a9071bf
Compare
baaddfe
to
0b411d6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're getting there – looks mostly good, one more nit.
90b5ec4
to
5beaa76
Compare
packages/dd-trace/src/config.js
Outdated
: isFalse(profilingEnabledEnv) | ||
? 'false' | ||
: profilingEnabledEnv === 'auto' ? 'auto' : undefined | ||
const profilingEnabled = normalizeProfilingEnabledValue(profilingEnabledEnv) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
profilingEnabledEnv
is only used by profilingEnabled
, should it be moved to the helper too? This would allow the helper to be used anywhere without having to know about the environment variables each time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can simplify the code a bit and remove profilingEnabledEnv
as an intermediate variable. Though the input of normalizeProfilingEnabledValue
isn't the same in the two places it's used (it's not an environment variable in stableConfig
) so we can't do what you suggest.
this.wasm_loaded = false | ||
|
||
if (isInServerlessEnvironment()) { | ||
// Bail out early if we're in a serverless environment, stable config isn't supported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be done before this file is ever loaded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried something in don't load config_stable.js on serverless; please tell me if this what you expect.
I'm semi-convinced by my approach because it forces us to check whether or not the stableConfig
object is defined at every call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No concerns on my end
What does this PR do?
Supports libdatadog's
library_config
module to configure the tracing libraries from a file on disk that may remotely managed.We don't load the WASM module if the files don't exist on disk.
Motivation
Being able to configure tracing libraries from a file / the UI
Additional Notes